Skip to content

Fix BC-break when using short-syntax notation for access_control#3113

Merged
soyuka merged 3 commits into
api-platform:2.5from
antograssiot:security-bc-break
Sep 27, 2019
Merged

Fix BC-break when using short-syntax notation for access_control#3113
soyuka merged 3 commits into
api-platform:2.5from
antograssiot:security-bc-break

Conversation

@antograssiot

@antograssiot antograssiot commented Sep 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

This BC-break as introduced in #2992

When using the accesControl attribute directly in the ApiRessource annotation, the AttributeHydratorTrait would throw the following:

Unknown property "accessControl" on annotation "ApiPlatform\Core\Annotation  \ApiResource".

core/src/Annotation/AttributesHydratorTrait.php

Lines 54 to 56 in 9799193

if (!property_exists($this, $key)) {
throw new InvalidArgumentException(sprintf('Unknown property "%s" on annotation "%s".', $key, self::class));
}

@teohhanhui

teohhanhui commented Sep 27, 2019

Copy link
Copy Markdown
Contributor

This must target 2.5 instead of master.

@antograssiot

antograssiot commented Sep 27, 2019

Copy link
Copy Markdown
Contributor Author

another option would be to update directly the AttributeExtractorTrait with something like, it would allow to trigger proper deprecation, WDYT ?

        if (array_key_exists('accessControl', $values)) {
            $values['security'] = $values['accessControl'];
            @trigger_error('Attribute "accessControl" is deprecated in annotation since API Platform 2.5, prefer using "security" attribute instead', E_USER_DEPRECATED);
            unset($values['accessControl']);
        }
        if (array_key_exists('accessControlMessage', $values)) {
            $values['securityMessage'] = $values['accessControlMessage'];
            @trigger_error('Attribute "accessControlMessage" is deprecated in annotation since API Platform 2.5, prefer using "securityMessage" attribute instead', E_USER_DEPRECATED);
            unset($values['accessControlMessage']);
        }

@antograssiot
antograssiot changed the base branch from master to 2.5 September 27, 2019 09:06
@teohhanhui

teohhanhui commented Sep 27, 2019

Copy link
Copy Markdown
Contributor

You should cherry pick your commit. We don't want the commits from master to go into 2.5.

@antograssiot

antograssiot commented Sep 27, 2019

Copy link
Copy Markdown
Contributor Author

You should cherry pick your commit. We don't want the commits from master to go into 2.5.

I know it was ongoing ;)

@antograssiot

antograssiot commented Sep 27, 2019

Copy link
Copy Markdown
Contributor Author

I've updated the test to add @group legacy to avoid the folllowing in PHPUNIT, but it is not directly related and I don't like it very much... is there any other way to silence them ?

Remaining self deprecation notices (4)

  2x: Attribute "accessControl" is deprecated in annotation since API Platform 2.5, prefer using "security" attribute instead
    1x in BooleanFilterTest::setUp from ApiPlatform\Core\Tests\Bridge\Doctrine\Orm\Filter
    1x in SwaggerCommandTest::testExecuteWithAliasVersion3 from ApiPlatform\Core\Tests\Bridge\Symfony\Bundle\Command

  2x: Attribute "accessControlMessage" is deprecated in annotation since API Platform 2.5, prefer using "securityMessage" attribute instead
    1x in BooleanFilterTest::setUp from ApiPlatform\Core\Tests\Bridge\Doctrine\Orm\Filter
    1x in SwaggerCommandTest::testExecuteWithAliasVersion3 from ApiPlatform\Core\Tests\Bridge\Symfony\Bundle\Command

@soyuka
soyuka merged commit 9a6853f into api-platform:2.5 Sep 27, 2019
@soyuka

soyuka commented Sep 27, 2019

Copy link
Copy Markdown
Member

thanks @antograssiot !

@antograssiot
antograssiot deleted the security-bc-break branch September 29, 2019 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@teohhanhui teohhanhui teohhanhui approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@antograssiot @teohhanhui @soyuka